-
Notifications
You must be signed in to change notification settings - Fork 2.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
38483 - fix edit message draft reappearing #39027
38483 - fix edit message draft reappearing #39027
Conversation
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
deb24b0
to
feb5df2
Compare
I have read the CLA Document and I hereby sign the CLA |
recheck |
1 similar comment
recheck |
@@ -221,6 +221,8 @@ function ReportActionItemMessageEdit( | |||
}, 1000), | |||
[reportID, action], | |||
); | |||
|
|||
useEffect(() => () => debouncedSaveDraft.cancel()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
useEffect(() => () => debouncedSaveDraft.cancel()) | |
useEffect(() => () => debouncedSaveDraft.cancel(), [debouncedSaveDraft]) |
If the dependency is not provided, the cleanup function can run multiple times. :)
@@ -280,7 +282,6 @@ function ReportActionItemMessageEdit( | |||
* Delete the draft of the comment being edited. This will take the comment out of "edit mode" with the old content. | |||
*/ | |||
const deleteDraft = useCallback(() => { | |||
debouncedSaveDraft.cancel(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can remove the debouncedSaveDraft
in the dependencies
Also, how about removing the cancel
in the publishDraft
at the same time? I think that's also stale.
App/src/pages/home/report/ReportActionItemMessageEdit.tsx
Lines 309 to 312 in 9dde2d5
// To prevent re-mount after user saves edit before debounce duration (example: within 1 second), we cancel | |
// debounce here. | |
debouncedSaveDraft.cancel(); |
…ies for useEffect and callbacks
Reviewer Checklist
Screenshots/VideosAndroid: Native39027-android-native.mp4Android: mWeb Chrome39027-android-chrome.mp4iOS: Native39027-ios-native.mp4iOS: mWeb Safari39027-ios-safari.mp4MacOS: Chrome / Safari39027-web.mp4MacOS: Desktop39027-desktop.mp4 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
BTW, this issue also occurs in the desktop app, so we can test it there as well. Also, since we're modifying common code, it would be better to test it on each platform to ensure the draft works correctly now. :)
cc @roitman-g
@ntdiary should I add videos of other platforms tests in the pull request? |
@ntdiary oh never mind, I can see you added those already, thank you |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Worked fine for me
It showed the error in the lint check here, which is why I made the last commit. But I might have been wrong. |
I see, sounds good |
@aldo-expensify looks like this was merged without a test passing. Please add a note explaining why this was done and remove the |
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
Removing |
🚀 Deployed to staging by https://github.com/aldo-expensify in version: 1.4.58-0 🚀
|
🚀 Deployed to production by https://github.com/Beamanator in version: 1.4.58-8 🚀
|
Details
When clicking up arrow key (to edit the last message) and escape (to cancel editing the last message) quickly, the popup to edit the message appears back again after a second.
This PR fixes this issue.
Fixed Issues
$#38483
PROPOSAL:$#38483 (comment)
Tests
Same as QA steps
Offline tests
Same as QA steps
QA Steps
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label and/or tagged@Expensify/design
so the design team can review the changes.ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Android: Native
N/A
Android: mWeb Chrome
N/A
iOS: Native
N/A
iOS: mWeb Safari
N/A
MacOS: Chrome / Safari
popupbug.1.1.mp4
MacOS: Desktop
N/A